-
Notifications
You must be signed in to change notification settings - Fork 1.3k
[roottest] Migrate Make to CMake #18596
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
roottest/oldMakefile
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From root-project/roottest#726 (comment):
Please remove this only as the very last step. It is still used(/useable) to clean all the by-product of the 'make' runs that are executed by ctest.
ROOT_LINKER_LIBRARY(TreeFormulaRetobjGeneration | ||
${CMAKE_CURRENT_SOURCE_DIR}/TreeFormulaRetobjGeneration.cxx | ||
G__TreeFormulaRetobjGeneration.cxx | ||
LIBRARIES Core Tree Hist MathCore) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From root-project/roottest#773 (comment):
Please add the generation of the dictionary as required fixture.
gSystem->Load("libEvent"); | ||
TFile *Event = TFile::Open("Event.new.split0.root"); | ||
gSystem->Load("libTreeFormulaRetobjGeneration"); | ||
TFile *Event = TFile::Open("Event.root"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be better to avoid this generic file name.
See the discussion in the original PR:
root-project/roottest#773 (comment)
if(MSVC) | ||
add_custom_command(TARGET IoCompressionGeneration POST_BUILD | ||
COMMAND ${CMAKE_COMMAND} -E copy ${CMAKE_CURRENT_BINARY_DIR}/$<CONFIG>/libIoCompressionGeneration.dll | ||
${CMAKE_CURRENT_BINARY_DIR}/libIoCompressionGeneration.dll | ||
COMMAND ${CMAKE_COMMAND} -E copy ${CMAKE_CURRENT_BINARY_DIR}/$<CONFIG>/libIoCompressionGeneration.lib | ||
${CMAKE_CURRENT_BINARY_DIR}/libIoCompressionGeneration.lib) | ||
endif() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From root-project/roottest#774 (comment):
@bellenot Is that an okay solution?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From root-project/roottest#774 (comment):
@bellenot Is that an okay solution?
Almost. It should be:
if(MSVC AND NOT CMAKE_GENERATOR MATCHES Ninja)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess, problem can be resolved using ROOTTEST_GENERATE_DICTIONARY
macro.
ROOT_LINKER_LIBRARY(IoCompressionGeneration | ||
${CMAKE_CURRENT_SOURCE_DIR}/IoCompressionGeneration.cxx | ||
G__IoCompressionGeneration.cxx | ||
LIBRARIES Core Tree Hist MathCore Physics Graf Matrix) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From root-project/roottest#774 (comment):
I recommend adding the ROOT_GENERATE_DICTIONARY
as a FIXTURES_REQUIRED
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is ROOTTEST_GENERATE_DICTIONARY
macro which supports FIXTURES_REQUIRED
.
Example can be found in roottest/root/io/json
sub-dir
ROOTTEST_GENERATE_EXECUTABLE(IoCompressionGenerator | ||
${CMAKE_CURRENT_SOURCE_DIR}/IoCompressionGenerator.cxx | ||
LIBRARIES Core RIO Net Tree Hist MathCore IoCompressionGeneration | ||
DEPENDS IoCompressionGeneration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From root-project/roottest#774 (comment):
This actually needs to be a FIXTURES_REQUIRED
rather than a DEPENDS.
roottest/root/io/perf/CMakeLists.txt
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should also consider this previous discussion on root/io/perf
:
Test Results 19 files 19 suites 3d 16h 29m 4s ⏱️ For more details on these failures, see this check. Results for commit a8cd512. ♻️ This comment has been updated with latest results. |
As another general reminder, one challenge in the migration from the |
This PR combines the following roottest PRs and re-applies to commits to the main repository, since
roottest
was merged into the main repo: